Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update scope of fixtures from hosts-content longrun test_inc_updates #14842

Merged

Conversation

vijaysawant
Copy link
Contributor

@vijaysawant vijaysawant commented Apr 19, 2024

Problem Statement

longrun/test_inc_updates.py has 2 tests, module scope fixture creates one organization and same organization has been used in two test cases, after uploading manifest second time test was failing due to below error message

test name: test_positive_incremental_update_time
Error:
['Owner has already imported from another subscription management application. The following conflicts were found: [ DISTRIBUTOR_CONFLICT ]']

Solution

changed scope of fixture of one test case so it will not use previously created organization from same test session.

Related Issues

N/A

PRT test Cases example

trigger: test-robottelo
pytest: tests/foreman/longrun/test_inc_updates.py

@vijaysawant
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/longrun/test_inc_updates.py

@vijaysawant vijaysawant added the No-CherryPick PR doesnt need CherryPick to previous branches label Apr 19, 2024
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6623
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/longrun/test_inc_updates.py --external-logging
Test Result : ================= 2 passed, 112 warnings in 1126.00s (0:18:46) =================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 19, 2024
@vijaysawant vijaysawant marked this pull request as ready for review April 19, 2024 14:26
@vijaysawant vijaysawant added the 6.14.z Introduced in or relating directly to Satellite 6.14 label Apr 19, 2024
@vijaysawant vijaysawant force-pushed the 6.14.z-longrun-update-fixtures branch from c526c4e to dfc3e15 Compare April 19, 2024 14:36
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 19, 2024
Copy link
Contributor

@synkd synkd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. The test failure is kind of weird, looks like a redirect loop for redhat.com? I can't reproduce this locally. I'll try rerunning the 3.12 code quality check.

Copy link
Contributor

@damoore044 damoore044 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice fix! I could not reproduce either

@shweta83 shweta83 removed the 6.14.z Introduced in or relating directly to Satellite 6.14 label Apr 22, 2024
@@ -231,7 +231,7 @@ def test_positive_noapply_api(


@pytest.mark.tier3
def test_positive_incremental_update_time(module_target_sat, module_sca_manifest_org):
def test_positive_incremental_update_time(target_sat, function_sca_manifest_org):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using sca enabled manifest?

Copy link
Contributor Author

@vijaysawant vijaysawant Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • During 6.14.z TFA, I find this test was failing due to reimporting of manifest in same orgnaization (created using module scope fixture)
  • I just want to keep *_sca_manifest_org as it is, so just changed scope of fixture.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vijaysawant I had the same question while reading this, but this issue should be mentioned in the PR description in problem/solution.
And could you be little more descriptive in PR title and commit messge, which could help you in future to identity the cause of this failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @Gauravtalreja1 , this PR initially open for testing purpose and wanted to see behaviour of when test running in Jenkins (as same changes/ same tests in 6.14 & 6.15 behave differently if I talk about test results from latest build, we @ColeHiggins2 and @vsedmik monitor robottelo logs closely and took decision to test changes remotely)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was if we use non-sca function scoped fixture then also it would have worked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's fine by me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameerpathan111 @shweta83, from what I understood from the subscription team, we should move away from the entitlement manifests/orgs rather sooner than later. So, if there is no specific reason to use the entitlement manifest (e.g. testing subscription attach etc.), then we should rather go SCA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was going on 6.14.z branch so to keep the things consistent, I suggested the above change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsedmik I understand but my main point was using a module-level fixture in both the tests, whether it's module_sca_manifest_org or module_entitlement_manifest_org. Anyway, it was more of a suggestion and I'm fine with the current approach too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameerpathan111 due to module scope fixture test setup fails- because it creates organization and reuse in another test, the second test case uploads newly created manifest and due to which it end up with en Error
['Owner has already imported from another subscription management application. The following conflicts were found: [ DISTRIBUTOR_CONFLICT ]']
May be I unable to explain it in correct way, @synkd @vsedmik @ColeHiggins2 were discussing this issue last week, so we took dicision to change scope of fixture (module to function) rather change functionality (sca to non-sca)

@vijaysawant vijaysawant requested a review from shweta83 April 22, 2024 07:12
@vijaysawant vijaysawant changed the title update fixtures name update scope of fixtures from hosts-content longrun test Apr 22, 2024
@vijaysawant vijaysawant changed the title update scope of fixtures from hosts-content longrun test update scope of fixtures from hosts-content longrun test_inc_updates Apr 22, 2024
@vijaysawant vijaysawant marked this pull request as draft April 25, 2024 07:56
@vijaysawant vijaysawant force-pushed the 6.14.z-longrun-update-fixtures branch from dfc3e15 to c4554ef Compare April 25, 2024 09:28
@vijaysawant
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/longrun/test_inc_updates.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6683
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/longrun/test_inc_updates.py --external-logging
Test Result : ================= 2 passed, 111 warnings in 1242.64s (0:20:42) =================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 25, 2024
@vijaysawant vijaysawant marked this pull request as ready for review April 25, 2024 10:35
@vsedmik vsedmik merged commit 9483c09 into SatelliteQE:6.14.z Apr 26, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-CherryPick PR doesnt need CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants